Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MZO-60] 플레이리스트 마크업 작업 #41

Merged
merged 10 commits into from
Jul 27, 2023
Merged

Conversation

haryung-lee
Copy link

@haryung-lee haryung-lee commented Jul 25, 2023

Jira Issue 번호

#60

작업 내용

  • playlist 모달 추가
  • body 태그 스타일 수정
  • portal position 변경 (absolute)

To Reviewer

  • playlist 공통 컴포넌트 구조를 이전과 바꿔봤습니다. 원래 store, hook에 작성해주어야 할 것을 /components/playlist에 전부 작성하는 방식으로 해보았습니다. 저번에도 루키랑 이런 폴더 구조에 대해 논의한 적이 있었는데 시험삼아 한번 시도해봤으니 확인 후 피드백 부탁드립니다!
  • 우선 /components/playlist 내에 store, type, hooks 로 분리시키진 않았습니다. 코드 양이 별로 되지 않아 usePlaylist 파일에 작성했는데 이와 관련해서 루키 의견이 궁금합니다!
  • playlist버튼 같은 경우, 헤더와 하단 플레이어 바 등에서 사용됩니다. 따라서 따로 컴포넌트로 분리시켰습니다.
image 스크린샷 2023-07-26 오전 3 59 31
  • 추후 framer motion이 사용되면 dnd도 추가할 예정입니다.

Checklist

  • Code Review 요청
  • PR 제목 commit convention에 맞는지 확인
  • Label 설정
  • Jira 코드 리뷰 상태로 변경

@haryung-lee haryung-lee added the ✨ Feature 기능 개발 label Jul 25, 2023
@haryung-lee haryung-lee self-assigned this Jul 25, 2023
@haryung-lee haryung-lee changed the title Feature/mzo 60 [MZO-60] 플레이리스트 마크업 작업 Jul 25, 2023
Comment on lines +38 to +48
images: {
// NOTE: 임시로 작성
remotePatterns: [
{
protocol: 'https',
hostname: 'via.placeholder.com',
port: '',
pathname: '/**',
},
],
},
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

next/image에서 외부 src를 가져오기 위해 임시로 설정해둔 코드입니닷

@@ -22,7 +22,7 @@ export default function RootLayout({
}) {
return (
<html lang="ko" className={`${pretendard.variable}`}>
<body className="bg-gray-500">
<body className="relative m-auto h-screen min-w-[360px] max-w-[480px] bg-gray-500">
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

모든 페이지에서 기본적으로 적용되어야 하는 사항이여서 상위 태그인 body 태그로 옮겼습니다.

DAY6
</p>
</>
) : null}
</div>
<div className="flex gap-8 items-center">
<div className="flex gap-[23px] items-center">
<div className="flex items-center gap-4">
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gap, svg크기 등 디자인 시안이 변동된 부분 반영했습니다.

Comment on lines +109 to +111
<AppPortal.Wrapper portalName="modal-portal">
<BottomMusicPlayer />
</AppPortal.Wrapper>
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사실 하단 플레이어의 경우 template으로 빼던가 해야할 것 같습니다. 우선 폴더구조가 명확히 잡혀있지 않아 임시로 플레이리스트에 같이 작성해두었습니다.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

각 페이지 마다 계속 해당 컴포넌트를 삽입하기 보다는 template으로 분리하는 게 좋아 보이네요. 가능하다면 그렇게 작업하는 것이 좋아 보입니다. 요구사항이 점점 늘어날수록 변수도 많아지네요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

맞습니다 아마 나중에 폴더구조가 잡히면 수정하지 않을까 싶어요. 그런데 고민인 부분은, 여기서 Playlist.tsx 페이지가 아닌 컴포넌트로 만들어서 따로 layout을 사용하기가 애매해질것 같긴합니다.. Playing 페이지에서는 하단 뮤직 플레이어바가 안보이고 플레이리스트에서는 보여져야 하는 상황이여서요

Comment on lines 87 to -56
zIndex: {
modal: 9999,
toast: 999,
emojiPicker: 199,
musicPlayer: 99,
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사실 다른 포탈이면 z-index는 설정하지 않아도 순서에 따라 원하는대로 배치가 될것 같습니다.

현재는

  <AppPortal.Provider portalName="player-portal" />
  <AppPortal.Provider portalName="emoji-picker-portal" />
  <AppPortal.Provider portalName="modal-portal" />
  <AppPortal.Provider portalName="toast-portal">

순으로 되어있는데 마지막에 작성된 포탈이 상위 레이어를 갖게 됩니다.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그러네요. 마지막으로 작성된 포탈이 더 높은 순위를 가진다라.. 신기하네요. 원인이 뭘까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

음 그냥 단순히 html 태그상에서 나중에 작성된 태그가 우선순위가 더 높은거랑 같은 맥락이지 않을까 싶어요!

@haryung-lee haryung-lee linked an issue Jul 25, 2023 that may be closed by this pull request
@@ -0,0 +1,5 @@
import Playlist from './Playlist';
Copy link
Contributor

@RookieAND RookieAND Jul 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

개인적으로는 코드의 양이 얼마 되지 않더라도 관심사에 따라 폴더를 분리하는 게 좋다고 생각합니다.
추후 코드의 양이 늘어나게 될 경우 파일 명으로는 각각의 파일이 어떤 역할을 하는지 구분하기 어려워질 것 같아요.
비록 지금은 적은 양이지만 추후 확장성을 고려했을 때는 내부에서 store, hooks를 구분짓는 게 좋다고 생각합니다.

추가적으로 현재 playlist 관련 내용물이 components 폴더에 있는데, 이는 저희가 이전에 이야기 했던 domains 폴더에 넣어서 명확하게 해당 내용들이 어떤 곳에서 쓰이는지를 분리하는 게 좋다고 생각해요. 하랭의 의견은 어떠신가요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

비록 지금은 적은 양이지만 추후 확장성을 고려했을 때는 내부에서 store, hooks를 구분짓는 게 좋다고 생각합니다.

좋습니다!! 수정하겠습니다~~

추가적으로 현재 playlist 관련 내용물이 components 폴더에 있는데, 이는 저희가 이전에 이야기 했던 domains 폴더에 넣어서 명확하게 해당 내용들이 어떤 곳에서 쓰이는지를 분리하는 게 좋다고 생각해요. 하랭의 의견은 어떠신가요?

아 요 부분은 Playlist의 경우 공통 컴포넌트라고 생각해서 components 폴더에 두긴 했는데.. 약간 애매한 부분이 있는것 같긴 하네요. domains폴더와 components 폴더를 확실히 구분해야할것 같아요. 저는 딱 한번쓰이는건 domain에, 두번 이상 쓰인다면 componet에 두는걸로 인식했었는데 어떻게 하는게 좋을까요??

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 하랭의 의견에 동의합니다

Copy link
Contributor

@RookieAND RookieAND left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

작업해주셔서 감사합니다 하랭.
논의가 필요한 사항 몇 가지를 달아두었습니다.


const useToastAtom = atom(
(get) => get(playlistAtom).isOpenPlaylist,
(get, set, type: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

개인 취향이긴 합니다만 현재는 플레이 리스트가 "열렸는지 아닌지" 만을 체크하는 상황이기 때문에 굳이 switch 문이 필요하지 않다고 생각해요.

setState 자체를 넘기는 게 아니라 usePlaylist 훅에서 플레이리스트 열림 여부를 제어하는 함수 2개를 반환하기 때문에 괜찮다고 생각합니다.

const useToastAtom = atom(
  (get) => get(playlistAtom).isOpenPlaylist,
  (get, set, isOpenPlaylist: boolean) => {
        set(playlistAtom, {
          isOpenPlaylist,
        });
    }
  },
);

// 중략...

  return {
    isOpenPlaylist,
    openPlaylist: () => setIsOpenPlaylist(true),
    closePlaylist: () => setIsOpenPlaylist(false),
  };

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 좋습니다! 사실 switch문을 사용한 이유는 저기서 추가로 togglePlaylist를 만들었어서 그랬는데 토글은 딱히 필요가 없을것 같아서 빼뒀었습니다.

루키가 제안주신 코드로 수정해둘게요!

@@ -43,17 +43,53 @@ module.exports = {
800: '#444444',
900: '#262626',
},
blue: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

정말 짱입니다.

Comment on lines +109 to +111
<AppPortal.Wrapper portalName="modal-portal">
<BottomMusicPlayer />
</AppPortal.Wrapper>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

각 페이지 마다 계속 해당 컴포넌트를 삽입하기 보다는 template으로 분리하는 게 좋아 보이네요. 가능하다면 그렇게 작업하는 것이 좋아 보입니다. 요구사항이 점점 늘어날수록 변수도 많아지네요.

@@ -29,6 +29,7 @@ const PortalProvider = ({
return (
<PortalContext.Provider value={portalList}>
<div
className="absolute top-0 left-0 w-full"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

div 생성 시 바로 스타일링 입혀주는 것도 제가 놓친 부분인데 채워주셔서 감사합니다.

import Image from 'next/image';
import React from 'react';

import Close from '@/assets/icons/close.svg';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[NIT]
저는 아이콘 관련 SVG를 import 할 때 접미사로 Icon 을 붙이는 편인데 이것도 통일하는 게 좋을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 좋아요! Icon 붙이는걸로 통일해요~~


interface PlaylistButtonProps
extends React.ButtonHTMLAttributes<HTMLButtonElement> {
className?: React.ComponentProps<'button'>['className'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

타입 진짜 야무지네요. 제가 아직 이런 식의 타입 사용이 부족한데 많이 배웁니다.

Copy link
Contributor

@RookieAND RookieAND left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, 저도 작업하면서 폴더 구조나 수정해야할 것들이 보이면 수정해두겠습니다.

@haryung-lee haryung-lee merged commit ddb2022 into develop Jul 27, 2023
1 check passed
@haryung-lee haryung-lee deleted the feature/MZO-60 branch July 27, 2023 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Feature 기능 개발
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MZO-60] 플레이리스트 페이지 작업
2 participants